feat: coinjoin promotion / demotion - #7052

Reviewing files that changed from the base of the PR and between a1c3afb and ffb2e13.

• src/coinjoin/client.cpp (12 hunks)
• src/coinjoin/client.h (3 hunks)
• src/coinjoin/coinjoin.cpp (6 hunks)
• src/coinjoin/coinjoin.h (2 hunks)
• src/coinjoin/common.h (1 hunks)
• src/coinjoin/server.cpp (3 hunks)
• src/net_processing.cpp (1 hunks)
• src/test/coinjoin_inouts_tests.cpp (4 hunks)
• src/wallet/coinjoin.cpp (3 hunks)
• src/wallet/wallet.h (3 hunks)

• GitHub Check: arm-linux-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: win64-build / Build source

src/wallet/wallet.h (3)

28-28: LGTM: Include addition is appropriate.

The inclusion of wallet/coincontrol.h is necessary to support the new SelectTxDSInsByDenomination overload at line 547, which uses CoinType as a parameter. The existing forward declaration of CCoinControl at line 124 is insufficient for the enum type.

---

547-547: LGTM: Overload supports denomination-based selection with coin type filtering.

The new overload extends the existing SelectTxDSInsByDenomination to accept a CoinType parameter, enabling more granular control over input selection for promotion/demotion workflows.

---

565-580: LGTM: Well-documented wallet APIs for promotion/demotion support.

The new methods provide essential functionality for the post-V24 promotion/demotion feature:

• CountCoinsByDenomination counts coins by denomination with optional fully-mixed filtering
• SelectFullyMixedForPromotion selects fully-mixed coins suitable for promotion

Both methods are correctly marked const and have clear documentation.

src/coinjoin/common.h (5)

112-114: LGTM: Clear promotion/demotion constants.

The constants are well-defined:

• PROMOTION_RATIO = 10 reflects the 10:1 ratio between adjacent denominations
• GAP_THRESHOLD = 10 defines the deficit threshold for triggering promotion/demotion

---

120-129: LGTM: Correct denomination index lookup.

The function correctly maps a bitshifted denomination to its index in vecStandardDenominations, returning -1 for invalid denominations. The logic and bounds checking are sound.

---

135-141: LGTM: Correct adjacency check for denominations.

The function correctly validates that two denominations are adjacent in the standard denomination list, handling invalid inputs appropriately. This is essential for promotion/demotion validation.

---

146-151: LGTM: Correct larger adjacent denomination lookup.

The function correctly navigates to the larger adjacent denomination, returning 0 when the input is already the largest or invalid. The bitshift calculation is accurate.

---

156-161: LGTM: Correct smaller adjacent denomination lookup.

The function correctly navigates to the smaller adjacent denomination, returning 0 when the input is already the smallest or invalid. The bounds checking and bitshift calculation are accurate.

src/coinjoin/server.cpp (4)

7-9: LGTM: Necessary includes for V24 deployment checks.

The includes are required:

• chainparams.h for accessing consensus parameters via Params()
• deploymentstatus.h for the DeploymentActiveAt function used in V24 activation checks

---

228-238: LGTM: Correct V24 gate for promotion/demotion entries.

The deployment gate correctly rejects unbalanced (promotion/demotion) entries when V24 is not active. The logic appropriately:

• Detects unbalanced entries by comparing input/output counts
• Checks V24 activation status at the current chain tip
• Rejects with ERR_MODE when the feature is not yet active

---

607-618: LGTM: Dynamic max inputs based on V24 activation.

The code correctly adjusts the maximum entry input count based on V24 activation:

• Pre-V24: limits to COINJOIN_ENTRY_MAX_SIZE (9)
• Post-V24: allows up to PROMOTION_RATIO (10) for promotion entries

The logging and error handling are appropriate. This change works in conjunction with the ProcessDSVIN gate to properly support promotion/demotion entries.

---

228-238: Note: V24 activation checks are performed separately.

The code checks V24 activation in both ProcessDSVIN (line 232) and AddEntry (line 610). While these are called sequentially and the risk is minimal, be aware that the chain tip could theoretically advance between checks. In practice, this is acceptable as it would only result in entry rejection, which is safe.

This is an informational note for awareness; verification via testing would confirm the behavior is correct under block transitions.

Also applies to: 607-618

src/coinjoin/coinjoin.h (2)

284-284: LGTM: Signature update for deployment-aware validation.

The updated IsValidStructure(const CBlockIndex* pindex) signature correctly enables V24 deployment detection for promotion/demotion validation. Using nullptr to indicate pre-fork behavior is a reasonable pattern.

---

367-388: LGTM: Well-documented validation helper declarations.

The new ValidatePromotionEntry and ValidateDemotionEntry declarations are properly documented with clear parameter semantics. The API correctly identifies the session denomination role (smaller denom for both cases) and returns validation status via the out-parameter.

src/wallet/coinjoin.cpp (3)

48-51: LGTM: Clean delegation for backward compatibility.

The existing 3-parameter overload now delegates to the new 4-parameter version with CoinType::ONLY_READY_TO_MIX as default. This preserves existing behavior while enabling the new coin type flexibility.

---

53-68: LGTM: New overload with configurable coin type.

The new overload correctly accepts CoinType and passes it to CCoinControl. The enhanced logging at line 68 is helpful for debugging coin selection with different coin types.

---

414-441: LGTM: CountCoinsByDenomination implementation is correct.

The function properly:

• Guards on CCoinJoinClientOptions::IsEnabled()
• Validates denomination via DenominationToAmount
• Uses depth check < 1 to skip unconfirmed/conflicted (consistent with line 462)
• Correctly handles the fFullyMixedOnly filter

src/coinjoin/client.h (4)

146-150: LGTM: Promotion/demotion session state additions.

The new session state members are properly initialized:

• m_fPromotion{false} and m_fDemotion{false} with brace initializers
• m_vecPromotionInputs as an empty vector

These flags enable session-level tracking of the entry type being prepared.

---

176-183: LGTM: Entry preparation methods with proper lock annotations.

Both PreparePromotionEntry and PrepareDemotionEntry correctly require m_wallet->cs_wallet lock, consistent with PrepareDenominate above. The separation of promotion (10→1) and demotion (1→10) preparation is clean.

---

335-350: LGTM: Manager-level decision helpers with clear documentation.

The ShouldPromote and ShouldDemote methods are properly documented with parameter semantics. These provide the decision logic for when to trigger promotion/demotion based on denomination counts and goals.

---

164-168: Verify SetNull() clears new state members.

The new parameters to JoinExistingQueue and StartNewQueue correctly extend the API with defaults for backward compatibility. Ensure SetNull() in the session class properly resets m_fPromotion, m_fDemotion, and m_vecPromotionInputs before merging.

src/test/coinjoin_inouts_tests.cpp (7)

52-86: LGTM: Updated tests for pindex-aware IsValidStructure.

The tests correctly pass nullptr to IsValidStructure() to test pre-V24 behavior, matching the updated signature. The test cases cover:

• Valid structure
• Invalid identifiers
• Size mismatch (pre-V24 rejection)
• Invalid scripts and amounts

Good coverage of pre-fork validation paths.

---

137-149: LGTM: Clean test helper functions.

MakeDenomInput and MakeDenomOutput provide reusable helpers for constructing test inputs/outputs. The helpers correctly use P2PKHScript for valid script generation.

---

151-175: LGTM: Valid promotion entry test.

The test correctly validates a promotion entry with:

• 10 inputs (PROMOTION_RATIO)
• 1 output at the larger adjacent denomination
• Proper denomination adjacency (0.1 → 1.0 DASH)

---

477-500: Critical test for value preservation across denominations.

This test validates a key invariant: nSmaller * PROMOTION_RATIO == nLarger for all adjacent denomination pairs. This ensures promotion/demotion preserves exact value.

---

722-756: Test helper mirrors implementation logic correctly.

The TestShouldPromote and TestShouldDemote helpers correctly implement the decision algorithm with:

• Half-goal threshold check
• Deficit calculation (max(0, goal - count))
• Gap threshold comparison

This enables testing the algorithm logic without wallet dependencies.

---

827-861: Thorough mutual exclusivity testing.

The promote_demote_mutually_exclusive test correctly validates that for any count distribution, at most one of promote/demote can be true. The use of structured bindings (auto [p, d] = ...) is clean C++17 style.

---

1019-1039: Good documentation of functional test requirements.

The comment block properly documents that post-V24 behaviors requiring EHF activation cannot be unit tested and require functional tests. This sets clear expectations for test coverage boundaries.

src/coinjoin/coinjoin.cpp (7)

9-9: LGTM: Required include for deployment status checks.

The deploymentstatus.h include is necessary for the DeploymentActiveAt calls used in V24 activation checks.

---

93-136: LGTM: IsValidStructure correctly handles V24 deployment.

The implementation correctly:

• Uses DeploymentActiveAt with pindex for V24 detection
• Pre-V24: Requires balanced vin/vout counts
• Post-V24: Allows unbalanced for promotion/demotion
• Dynamically adjusts max inputs (180 pre-fork, 200 post-fork)
• Validates all outputs are denominated and P2PKH

The comment about value sum validation being deferred to IsValidInOuts is accurate since it requires UTXO access.

---

221-260: LGTM: Entry type detection with proper V24 gating.

The entry type detection logic correctly:

• Identifies STANDARD (balanced) entries for all versions
• Only allows PROMOTION/DEMOTION post-V24
• Properly rejects invalid structures with ERR_SIZE_MISMATCH

The local EntryType enum provides clear semantics.

---

261-288: LGTM: Denomination expectations set correctly per entry type.

For each entry type:

• STANDARD: inputs and outputs at session denom
• PROMOTION: inputs at session denom, output at larger adjacent
• DEMOTION: input at larger adjacent, outputs at session denom

The early failure when nLargerAdjacentDenom == 0 prevents invalid promotion/demotion from the largest/smallest denominations.

---

351-363: LGTM: Value preservation check applies to all entry types.

The nFees != 0 check correctly ensures total input value equals total output value. This is essential for promotion/demotion where 10 × smaller_denom == larger_denom must hold exactly.

The logging now includes entry type, which aids debugging.

---

583-633: LGTM: ValidatePromotionEntry implementation is correct.

The function properly validates:

• Exactly PROMOTION_RATIO inputs
• Exactly 1 output
• Larger adjacent denomination exists
• Output matches larger denomination
• Output is P2PKH

Note: Input denomination validation is not performed here as this function validates structure, not UTXO values (done in IsValidInOuts).

---

635-675: LGTM: ValidateDemotionEntry correctly validates structure.

The function properly validates:

• Exactly 1 input
• Exactly PROMOTION_RATIO outputs
• All outputs at session denomination
• All outputs are P2PKH

Similar to promotion, input denomination validation is deferred to IsValidInOuts where UTXO access is available.

src/coinjoin/client.cpp (11)

11-11: LGTM!

The new includes for deploymentstatus.h and validation.h are appropriate for the V24 deployment detection logic added in this PR.

Also applies to: 25-25

---

295-299: LGTM!

Proper cleanup of the new promotion/demotion session state fields, with the lock assertion already in place from the existing code.

---

979-1035: Clarify target denomination semantics for queues.

The V24 detection and iteration logic looks correct. However, I want to verify the target denomination semantics:

• For promotion (lines 1007, 1011): nSmallerDenom is passed as target - this makes sense as the queue is for the smaller denomination that gets promoted to larger.
• For demotion (lines 1026, 1030): nSmallerDenom is also passed - this means the queue denomination is the output denomination, not the input.

This is consistent with the queue matching in JoinExistingQueue at line 1185 which checks dsq.nDenom != nTargetDenom. The session denomination represents what the mixing session produces, which for demotion is the smaller denomination.

The logic is correct but the naming could be clearer. The nTargetDenom effectively means "the denomination this session is operating on" which works for both promotion inputs and demotion outputs.

---

1575-1594: LGTM!

Clean separation of promotion/demotion submission paths from standard mixing. The error handling properly logs failures and sets the status message.

---

1725-1784: LGTM!

The promotion entry preparation correctly implements the 10:1 ratio:

• 10 inputs from fully-mixed smaller denomination coins
• 1 output of the larger adjacent denomination
• Empty CTxOut() placeholders are properly filtered out in SendDenominate
• Input locking prevents double-spending during the session

---

1786-1845: LGTM with a note on denomination semantics.

The demotion entry preparation correctly implements the 1:10 ratio:

• 1 input from the larger denomination coin
• 10 outputs of the smaller denomination
• Empty CTxDSIn() placeholders are properly filtered in SendDenominate

The denomination logic is correct: nSessionDenom is the target denomination (smaller, for outputs), and GetLargerAdjacentDenom(nSessionDenom) correctly identifies the input denomination for validation.

---

2316-2349: Well-designed promotion decision logic.

The algorithm correctly:

1. Validates denomination adjacency
2. Protects denominations still being built (< half goal)
3. Uses a gap threshold to prevent oscillation
4. Requires fully-mixed coins to maintain anonymity guarantees

The hysteresis via GAP_THRESHOLD is a good design choice to prevent thrashing between promotion and demotion.

---

2351-2378: LGTM!

The demotion decision logic correctly mirrors promotion but without the fully-mixed requirement. This asymmetry is intentional and makes sense:

• Promotion requires fully-mixed coins to preserve anonymity (10 mixed → 1 mixed)
• Demotion can use any denominated coin since splitting doesn't reduce anonymity (1 → 10 new outputs)

The gap threshold hysteresis is shared, preventing oscillation.

---

1192-1241: Verify wallet lock consistency between promotion and demotion paths.

For the promotion path (lines 1192-1213), you explicitly acquire m_wallet->cs_wallet at line 1200 to safely access mapWallet. However, for the demotion path (lines 1214-1234), you call SelectTxDSInsByDenomination without an explicit lock in this scope. Confirm whether SelectTxDSInsByDenomination acquires the wallet lock internally or if explicit locking is needed here to match the promotion path's pattern.

---

1576-1594: Resource cleanup on preparation failure follows existing timeout-based pattern.

In the promotion/demotion paths (lines 1577–1594), if PreparePromotionEntry or PrepareDemotionEntry fails, locked coins remain locked until the session times out and calls SetNull() → UnlockCoins(). This matches the pattern used in standard mixing paths. Consider whether immediate cleanup would improve user experience by freeing coins sooner, though the current approach is consistent with the codebase design.

---

499-510: Remove references to non-existent preparation functions and correct CTxOut behavior.

The filtering logic uses prevout.IsNull() and nValue > 0 to distinguish placeholder entries from real transaction data. However, the original review contains factual errors:

• CTxOut's default constructor calls SetNull(), setting nValue to -1 (not 0), so the check if (txOut.nValue > 0) correctly filters out both default (-1) and zero-value outputs.
• The referenced functions PreparePromotionEntry and PrepareDemotionEntry do not exist in the Dash Core repository and cannot be verified against this filtering logic.

The actual filtering appears sound for handling asymmetric input/output counts in promotion/demotion scenarios, but the justification in the original comment is incorrect.

Likely an incorrect or invalid review comment.

Reviewing files that changed from the base of the PR and between ffb2e13 and d140732ef87897c32385398fa7dca34fa4784091.

• src/coinjoin/client.cpp (12 hunks)
• src/coinjoin/client.h (3 hunks)
• src/coinjoin/coinjoin.cpp (6 hunks)
• src/coinjoin/coinjoin.h (4 hunks)
• src/coinjoin/common.h (1 hunks)
• src/coinjoin/server.cpp (4 hunks)
• src/net_processing.cpp (1 hunks)
• src/test/coinjoin_inouts_tests.cpp (4 hunks)
• src/wallet/coinjoin.cpp (3 hunks)
• src/wallet/wallet.h (3 hunks)

• src/net_processing.cpp
• src/coinjoin/common.h
• src/wallet/wallet.h

• GitHub Check: linux64-test / Test source
• GitHub Check: linux64_sqlite-test / Test source
• GitHub Check: linux64_ubsan-test / Test source
• GitHub Check: linux64_tsan-test / Test source
• GitHub Check: linux64_multiprocess-build / Build source

src/wallet/coinjoin.cpp (3)

48-51: LGTM: Clean delegation pattern.

The 4-parameter overload correctly delegates to the new 5-parameter version with the appropriate default CoinType::ONLY_READY_TO_MIX, maintaining backward compatibility.

---

53-93: LGTM: CoinType parameterization enables promotion/demotion flows.

The new 5-parameter overload correctly propagates the CoinType to CCoinControl and includes it in logging. The implementation maintains the original coin selection logic while adding flexibility for different coin type filtering.

---

414-441: Verify depth check semantics.

The depth check GetTxDepthInMainChain(it->second) < 1 filters unconfirmed or conflicted transactions. According to GetTxDepthInMainChain semantics, this function returns positive depth for confirmed transactions, 0 for unconfirmed, and negative depth for conflicted transactions. The < 1 check correctly keeps only confirmed transactions with depth >= 1.

src/coinjoin/server.cpp (3)

228-238: LGTM: Correct V24 deployment gating for promotion/demotion.

The pre-check correctly identifies unbalanced entries (promotion/demotion) and rejects them when V24 is not active. The use of ERR_MODE is appropriate for incompatible mode rejection, and the null pindex check prevents potential crashes.

---

303-306: LGTM: Critical privacy protection for promotion/demotion flows.

The change from GetEntriesCount() to GetStandardEntriesCount() is essential for privacy. Promotion/demotion entries should not count toward the minimum participant threshold because they don't contribute to the anonymity set—they rely on standard mixing participants for privacy. The inline comment clearly explains this rationale.

---

609-620: LGTM: Deployment-aware entry input limits.

The V24-aware maximum input calculation correctly increases the limit to PROMOTION_RATIO (10) for post-V24 promotion entries while maintaining the pre-V24 limit of COINJOIN_ENTRY_MAX_SIZE (9). The error logging includes the actual limit for clarity.

src/coinjoin/coinjoin.h (4)

172-179: LGTM: Clear entry type classification.

The IsStandardMixingEntry() method provides a simple and correct classification based on input/output balance. The inline implementation is appropriate for this trivial check, and the comment clearly explains the three entry types.

---

329-341: LGTM: Correct lock annotations and idiomatic implementation.

The standard entry counting methods correctly use thread-safety annotations to prevent deadlocks. The locked variant (GetStandardEntriesCountLocked) is provided for callers already holding the lock, and the implementation uses idiomatic std::count_if. The comment clearly explains the privacy threshold purpose.

---

391-411: LGTM: Well-documented validation API.

The promotion/demotion validation functions are cleanly declared in the CoinJoin namespace with comprehensive documentation. The parameter list is appropriate for the validation task, and the pattern of returning bool with an out-parameter nMessageIDRet is consistent with existing code.

---

293-293: All call sites have been properly updated for the signature change. The production code in src/net_processing.cpp:3532 passes the block index parameter, while test code appropriately passes nullptr for pre-fork validation testing.

src/test/coinjoin_inouts_tests.cpp (5)

52-87: LGTM: Correct pre-fork testing pattern.

The tests correctly use nullptr for the pindex parameter to simulate pre-V24 behavior. This aligns with the implementation in IsValidStructure where !fV24Active results in pre-fork validation rules. The comments clearly document this pattern.

---

138-149: LGTM: Clean test helper utilities.

The MakeDenomInput and MakeDenomOutput helper functions provide clean abstractions that reduce test boilerplate while ensuring unique prevouts and correct P2PKH scripts as required by CoinJoin.

---

469-492: LGTM: Critical value preservation invariant.

This test verifies the fundamental invariant that 10 * smaller == larger exactly for all adjacent denomination pairs. This is essential for promotion/demotion to preserve value without fees. The test correctly covers all adjacent pairs in the denomination ladder.

---

684-856: LGTM: Comprehensive promotion/demotion decision algorithm tests.

The test suite thoroughly covers the promotion/demotion decision algorithms:

• HalfGoal threshold prevents sacrificing denominations being built up
• GAP_THRESHOLD ensures sufficient deficit gap before action
• Mutual exclusivity tests confirm at most one action triggers
• Specific examples from the implementation plan are verified

These tests provide confidence in the correctness of the decision logic that will balance denomination distributions.

---

982-1021: LGTM: Privacy threshold counting verification.

This test verifies that GetStandardEntriesCount correctly excludes promotion/demotion entries from the count, returning only the standard mixing entries (3 of 5 total). This aligns with the privacy protection logic in the server where only standard entries count toward the minimum threshold.

src/coinjoin/client.h (4)

164-168: LGTM: Clean API extension with backward compatibility.

The queue method signatures are extended with default parameters (nTargetDenom = 0, fPromotion = false, fDemotion = false), maintaining backward compatibility while enabling promotion/demotion flows. The overloading pattern keeps the API clean.

---

177-183: LGTM: Well-documented preparation methods with correct lock annotations.

The PreparePromotionEntry and PrepareDemotionEntry methods are properly annotated with EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) to ensure thread safety. The API follows the existing pattern of returning bool with a string error message and output parameters for the constructed entry pairs.

---

336-350: LGTM: Clean decision API with comprehensive documentation.

The ShouldPromote and ShouldDemote methods provide a clean decision API. They are correctly marked const since they only inspect state without modification. The comprehensive documentation clearly explains the parameters and return values.

---

146-149: Mutual exclusivity of promotion/demotion flags is enforced.

The implementation correctly ensures mutual exclusivity:

• JoinExistingQueue includes an explicit assertion: assert(!(fPromotion && fDemotion)) at line 1166
• StartNewQueue enforces it through control flow with if (fPromotion) { ... } else if (fDemotion) { ... } else { return false; } at lines 1400-1444
• All callers pass mutually exclusive values (one true, one false)

src/coinjoin/coinjoin.cpp (4)

93-134: LGTM: Correct V24-aware structure validation.

The IsValidStructure implementation correctly:

• Computes V24 activation via DeploymentActiveAt with null pindex handling
• Rejects unbalanced entries pre-V24 (line 104)
• Adjusts max inputs: 200 post-V24 vs 180 pre-V24 (lines 114-116)
• Validates all outputs are valid denominations and P2PKH (lines 123-127)
• Documents that value sum validation is deferred to IsValidInOuts (lines 129-131)

The implementation aligns with the test patterns that use nullptr for pre-fork scenarios.

---

210-363: LGTM: Comprehensive entry validation with proper V24 support.

The extended IsValidInOuts implementation provides thorough validation:

1. V24 activation check (lines 220-225): Correctly acquires cs_main to access chain tip
2. Entry classification (lines 227-257): Properly categorizes STANDARD/PROMOTION/DEMOTION/INVALID
3. Denomination validation (lines 259-285): Correctly determines expected denoms based on entry type
4. Script validation (lines 287-311): Validates P2PKH requirement and prevents duplicate scripts
5. UTXO validation (lines 324-347): Ensures inputs exist and are spendable
6. Fee enforcement (lines 349-355): Correctly requires zero fees for CoinJoin
7. Logging (lines 357-362): Includes entry type for debugging

The implementation correctly handles the complexity of three entry types while maintaining the core CoinJoin guarantees.

---

582-631: LGTM: Thorough promotion entry validation.

The ValidatePromotionEntry implementation correctly validates:

• Input count must be exactly PROMOTION_RATIO (10)
• Output count must be exactly 1
• Larger adjacent denomination must exist (not largest)
• Output denomination must match the larger adjacent denomination
• Output must be P2PKH script

Error messages are descriptive and use LogPrint for debugging. The validation prevents malformed promotion entries from entering the pool.

---

633-673: LGTM: Complete demotion entry validation.

The ValidateDemotionEntry implementation correctly validates:

• Input count must be exactly 1
• Output count must be exactly PROMOTION_RATIO (10)
• All outputs must be at session denomination (smaller)
• All outputs must be P2PKH scripts

The loop at lines 657-670 correctly validates all outputs, not just the first, ensuring the entire demotion structure is valid.

src/coinjoin/client.cpp (6)

11-11: LGTM!

The new includes support V24 deployment detection and chain state access required by the promotion/demotion logic.

Also applies to: 25-25

---

510-525: LGTM! Filtering approach correctly handles promotion/demotion entry structure.

The filtering removes empty placeholder inputs/outputs from promotion (10 inputs → 1 output) and demotion (1 input → 10 outputs) entries before relay. The updated log reflects actual counts submitted.

---

991-1045: Verify denomination promotion/demotion priority order is intentional.

The loop processes adjacent denomination pairs in order i=0 to i=3, which corresponds to:

• i=0: 10 DASH ↔ 1 DASH
• i=1: 1 DASH ↔ 0.1 DASH
• i=2: 0.1 DASH ↔ 0.01 DASH
• i=3: 0.01 DASH ↔ 0.001 DASH

Since the function returns on first successful promotion/demotion (lines 1019, 1023, 1037, 1041), larger denominations have priority. Confirm this matches the intended behavior, or consider processing in reverse order to prioritize completing smaller denominations first (which may be more common operations).

---

1598-1617: LGTM! Clear separation of promotion/demotion and standard mixing flows.

The conditional logic correctly routes to PreparePromotionEntry/PrepareDemotionEntry for V24 flows and falls back to standard mixing otherwise. Error handling is appropriate.

---

1768-1794: Add bounds checking and remove redundant IsSpent check.

Line 1775 accesses wtx.tx->vout[outpoint.n] without validating bounds (same issue appears at line 1846 in PrepareDemotionEntry).

Additionally, the IsSpent check at lines 1781-1784 is redundant since coins in m_vecPromotionInputs are already locked (preventing spending). If a locked coin is spent, that indicates a critical wallet inconsistency that should be handled differently (assertion or stronger error).

🔎 Add bounds check and consider removing redundant IsSpent

         const wallet::CWalletTx& wtx = it->second;
+        if (outpoint.n >= wtx.tx->vout.size()) {
+            strErrorRet = "Invalid promotion input index";
+            return false;
+        }
 
-        // Validate the UTXO is still spendable
-        if (m_wallet->IsSpent(outpoint)) {
-            strErrorRet = "Promotion input has been spent";
-            return false;
-        }
+        // Locked coins should never be spent - assert if this occurs
+        assert(!m_wallet->IsSpent(outpoint));

Apply similar changes to PrepareDemotionEntry at lines 1846-1855.

⛔ Skipped due to learnings

Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs

---

295-311: SetNull() properly handles promotion/demotion input unlocking with appropriate defensive checks.

The code correctly identifies and addresses a real tracking gap: promotion/demotion inputs are locked via m_wallet->LockCoin() in JoinExistingQueue/StartNewQueue but are not added to vecOutPointLocked until PreparePromotionEntry/PrepareDemotionEntry. Early session failures (connection issues, timeouts, validation failures) that call SetNull() before these preparation methods would leave inputs locked in the wallet but missing from vecOutPointLocked. The SetNull() code transfers m_vecPromotionInputs to vecOutPointLocked before clearing state, ensuring UnlockCoins() will properly unlock them. The duplicate check is necessary and defensive—it prevents adding the same outpoint multiple times to vecOutPointLocked, which could occur if the code path executes multiple times or encounters edge cases where inputs exist in both sources. This implementation is sound.

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1f8ea791-13b5-43f4-875c-8f47542e04b0

Reviewing files that changed from the base of the PR and between d140732ef87897c32385398fa7dca34fa4784091 and bc4b034.

• src/coinjoin/client.cpp
• src/coinjoin/client.h
• src/coinjoin/coinjoin.cpp
• src/coinjoin/coinjoin.h
• src/coinjoin/common.h
• src/coinjoin/server.cpp

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ccc108f2-8300-41e8-b074-26afc4c78c13

Reviewing files that changed from the base of the PR and between c2089b6 and 332186c.

• src/coinjoin/client.cpp
• src/coinjoin/client.h
• src/coinjoin/coinjoin.cpp
• src/coinjoin/server.cpp
• src/wallet/coinjoin.cpp